Skip to content

npu: count a gated q_proj's 2*QD rows, so check_q4nx_dir stops calling every attn_output_gate pack padded (Fixes #1285) - #1286

Merged
bong-water-water-bong merged 16 commits into
mainfrom
1bit/1285-gated-qproj-grid
Nov 6, 2026
Merged

bong-water-water-bong merged 16 commits into
mainfrom
1bit/1285-gated-qproj-grid

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Fixes #1285 (found while closing out the pack side of #796).

tools/check_q4nx_dir.py derives each weight's expected tile grid from config.json, and for
self_attn.q_proj.weight it expected num_attention_heads * head_dim rows. With attn_output_gate the
stored projection emits q and the output gate, so it holds 2 * that many rows — so the geometry check
reported a phantom 512-padded grid and --strict exited 1 on correct containers. That is the inverse of what
the tool is for: it told an operator to distrust a pack that decodes and answers.

Measured (header reads only, no device, no ticket), before → after

tools/check_q4nx_dir.py <dir> --strict over all 18 Q4NX model directories on this box:

container attn_output_gate WARN before WARN after rc before rc after
d3-lite-conv (the real vllm-sr/d3-lite pack, #796) true 6 (the 6 full-attention layers) 0 1 0
bonsai2-27b-NPU, -NPU-b8, -pb16i, -pb16i-unrot, -t2s2, -t2s2-serve, bonsai2-t2s2-perrole876 1 16 each 0 1 0
the other 10 dirs (Qwen3-0.6B ×3, ZR1-1.5B ×4, d3-lite-bf16, d3-synth-0.6b, qwen3-coder-30b-a3b-moe) absent 0 0 0 0

8 of 18 containers change; none gains a warning.

The container is right (two independent checks)

Change (2 files, 31+/5−)

  • dims_from_config carries QG = 2*QD when config.json sets attn_output_gate (JSON true or 1 — the
    forward reads it with q4nx_config_int, so a bool and an int must mean the same thing); expected_rc uses
    QG for q_proj. o_proj is unchanged: a gated layer's attention output is still QD wide, and the d3
    container holds exactly (H, QD) there.
  • A self-test case (ctest q4nx_dir_check) in both directions: a d3 q_proj of [512, 5120] tiles is
    quiet under gated dims, and the same table still warns under ungated dims — so the case only passes when the
    gate is what suppresses the warning.
  • One sentence in docs/npu.md, where the checker is described. (Not the noncausal_full_attention sentence —
    that one is in the open npu: the forward is causal-only, so a noncausal_full_attention pack (d3-lite) cannot be reproduced — measured, not inferred #847 PR.)

Verification

  • tools/check_q4nx_dir.py --self-test → passes; tested the test: with qg = qd restored it fails on
    attn_output_gate did not double q_proj's rows: {'QD': 2048, 'QG': 2048, …}, restored it passes.
  • The 18-directory sweep above, old and new script side by side; --strict rc 1 → 0 on both gated families.
  • A freshly converted container, not just the shipped one: tools/safetensors_to_q4nx.py pack ~/models/npu/d3-lite-src … --arch qwen3_5 → 320 tensors / 820,971,656 B, weight bytes byte-identical to
    the shipped d3-lite-conv (sha256 313ac4f9…), verify 320/320 ok, the three GDN transforms still exact
    on real weights (ssm_a = -exp(A_log) 18/18, conv1d tap-major 18/18 — a flatten differs in 18/18 —
    alpha/beta zero-padded 18/18), decision → decision.q4nx [255, 1024] f32. Old checker: 6 WARN / rc 1. New
    checker: 0 WARN / rc 0.
  • tools/copyright.py --check rc 0. Device never opened in any of the above.

bong-water-water-bong and others added 3 commits November 6, 2026 16:02
…g every attn_output_gate pack padded

`dims_from_config` expected `num_attention_heads * head_dim` rows for `self_attn.q_proj.weight`,
but with `attn_output_gate` the stored projection emits q AND the output gate (2*QD rows). The
mismatch made the geometry check report a 512-padded grid and fail `--strict` on 8 of the 18 Q4NX
directories on this box -- including the real d3 pack (#796) with 6 warnings and every 27B
qwen3_5 directory with 16 -- for containers that decode and answer correctly.

`QG = 2*QD` when config.json sets `attn_output_gate` (JSON true or 1, as the forward's
q4nx_config_int reads it); `expected_rc` uses it for q_proj. Self-test gains the d3 case in both
directions (quiet when gated, still warning when not), and docs/npu.md says so where the checker
is described.

Fixes #1285

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 39bead7a — reproduced on the real 27B pack this box holds: the shipped checker calls a correct container broken (rc 1 on main), and this PR makes the same directory pass, while a non-gated container behaves identically to main.

Before/after, same directory, same command, only the tool differs (~/models/npu/bonsai2-27b-pb16i, whose config.json sets attn_output_gate; the pack the 313/410 ms rows in the leaderboard are about):

origin/main tool:  FAIL model.layers.47.self_attn.q_proj.weight: holds 7680 tiles, the model's dimensions
                        imply 3840 - columns padded 5120 -> 10240 (the 512-column grid) … (engine#1097)   [×6 layers]
                   chunk layout NOT verified                                                          rc 1
this PR's tool:    chunk layout verified: every quantized tensor's shape names a known chunk kind          rc 0

7680/3840 = 2, which is the whole point: a gated q_proj emits q and the output gate, so its rows are 2 * num_attention_heads * head_dim. The old code used QD and then explained a correct grid as padding — i.e. --strict failed on the real container, which is exactly the class of false red the check exists to prevent.

The negative side is unchanged: ~/models/npu/d3-synth-0.6b/ (no attn_output_gate) gives chunk layout verified … at both heads, so the new branch is inert for the containers that do not need it.

The self-test grew the arm and the anti-vacuity control for it: python3 tools/check_q4nx_dir.py --self-test rc 0 with … and a gated (attn_output_gate) q_proj's 2*QD rows are the model's own grid — and the arm refuses to pass silently, because it also asserts the same tensor shape does warn when attn_output_gate is absent ("the same q_proj is quiet without attn_output_gate, so the case proves nothing"). That is the right shape: a case that cannot fail would be worthless here.

Two details I checked because they are how this breaks later:

  • the boolean is read the way the forward reads it — a JSON true or 1 (gate is True or gate == 1), matching q4nx_config_int, so a checkpoint that spells it as an integer is not silently treated as ungated;
  • QG is carried in the same dims_from_config() dict the rest of the expectations use, so q_proj and o_proj stay consistent (o_proj remains (H, QD) — its input is q's pre-gate width, and the diff correctly leaves it alone).

Docs are in the PR (docs/npu.md names the rule and both issues); tool + self-test + doc in one change.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at acac5938 — main merges only; the reviewed content is byte-identical to the head I verdicted.

Checked rather than assumed: the PR's own added lines (own diff vs merge-base, which excludes everything main brought in) hash to 260cec477c38 at the head I reviewed and to the same value here, so no line of this PR's change moved — only its base did (the branch now sits on newer main).

Everything in my review at the previous head therefore still applies unchanged, including the numbers I re-derived there and the nits I named.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 4f33529cceadac32758f1672d90d369ed663bf88. A small, correct fix, tested in both directions, and the 18-directory claim reproduces on a wider set.

Checked here (host only, no device)

  • Scope: git diff --stat $(merge-base) 4f33529c is 2 files, +31/−5 (tools/check_q4nx_dir.py, docs/npu.md), as the body says. MERGEABLE.
  • tools/check_q4nx_dir.py --self-test at the head: passes, rc 0.
  • I tested the test: with qg = qd * 2 if mutated to qg = qd if, the self-test fails with attn_output_gate did not double q_proj's rows: {'QD': 2048, 'QG': 2048, …}, rc 1. The case also asserts that the same table still warns under ungated dims, so it cannot pass vacuously.
  • The checker now agrees with the forward. npu/forward/src/q4nx_forward.cpp:757 reads attn_output_gate from config.json with q4nx_config_int, and q4nx_pack.h:187-192 documents q_proj as q|gate. Treating JSON true and 1 alike matches that. o_proj stays (H, QD), which is right: the gate is consumed before o_proj.
  • Sweep, old (origin/main) vs new, --strict, over every model.q4nx directory under ~/models, ~/lb and ~/stage-c26. That is 31 directories, a superset of the body's 18:
dir gate WARN rc
models/npu/d3-lite-conv true 6 → 0 1 → 0
lb/i796-fresh, lb/i796-fresh-noplan true 6 → 0 1 → 0
bonsai2-27b-NPU, -NPU-b8, -pb16i, -pb16i-unrot, -t2s2, -t2s2-serve, bonsai2-t2s2-perrole876 1 16 → 0 each 1 → 0
models/npu/d3-lite-bf16 true 0 → 0 0 → 0
the other 19 absent unchanged unchanged

No directory gains a warning or changes rc in the wrong direction.

  • The new docs/npu.md sentence is in the same PR as the behaviour. copyright.py --check rc 0.

Notes, not blocking

  • gate is True or gate == 1 treats any other nonzero int as ungated, while the forward treats any nonzero as gated (if (attn_gate_)). No config on the box uses another value, but bool(gate) (excluding strings) would match the forward exactly.
  • #1275 edits the same file (the self-test tail and the success line). The two may conflict textually when the second one lands; they don't conflict logically.

Fixes #1285 is accurate: the false WARN / --strict rc 1 on gated containers is gone, measured.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at ecebe03510f05736e0c6adf4eaa30fd506417fab. Main merges only since my READY at 4f33529c: the PR diff's git patch-id --stable against its merge-base is identical (298bfe339e36) at both heads, and there are no non-merge commits of its own. Re-checked at this head: tools/check_q4nx_dir.py --self-test rc 0. The full review at 4f33529c stands.

@bong-water-water-bong
bong-water-water-bong merged commit 7082eaa into main Nov 6, 2026
9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the 1bit/1285-gated-qproj-grid branch November 6, 2026 21:42
@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1285 - Partially compliant

Compliant requirements:

  • Fix the tools/check_q4nx_dir.py script to correctly handle gated attention layers
  • Update documentation to reflect the change in how gated q_proj dimensions are calculated
  • Verify that the fix resolves the issue where --strict mode exits 1 on correct containers
  • Ensure that the self-test in check_q4nx_dir.py validates the new behavior

Non-compliant requirements:

  • None

Requires further human verification:

  • None
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Incorrect q_proj dimension calculation for gated attention

The tool incorrectly calculates the expected dimensions for q_proj in gated attention layers. For gated layers, q_proj should have 2 * QD rows, but the tool was using QD rows, causing it to incorrectly flag correct containers as having padded grids. This has been fixed by introducing QG (gated QD) and using it for q_proj in expected_rc.

if name == "lm_head.weight":
    return d["V"], d["H"]
tail = name.split(".", 3)[3] if name.startswith("model.layers.") and name.count(".") > 3 else ""
return {
    "self_attn.q_proj.weight": (d["QG"], d["H"]),
Missing test case for gated q_proj

The self-test in check_q4nx_dir.py now includes a specific test case for gated attention layers to ensure that q_proj with attn_output_gate correctly uses 2 * QD rows and does not produce warnings when it should not.

# A gated attention layer's q_proj holds 2*QD rows (attn_output_gate, engine#796): the real d3
# pack's q_proj is 512 tiles of 32x256, i.e. 4096x1024 rows and cols. Reading it as QD rows
# made the shipped d3-lite-conv warn "columns padded 1024 -> 2048" on all six full-attention
# layers and fail --strict, i.e. the checker called a correct container broken.
d3cfg = {"hidden_size": 1024, "num_attention_heads": 8, "num_key_value_heads": 2,
         "head_dim": 256, "intermediate_size": 3584, "vocab_size": 248320}
gated = dims_from_config(dict(d3cfg, attn_output_gate=True))
plain = dims_from_config(d3cfg)
qproj = {"model.layers.3.self_attn.q_proj.weight": {"dtype": "I8", "shape": [512, 5120]}}
if not gated or gated["QG"] != 2 * gated["QD"]:
    bad.append(f"attn_output_gate did not double q_proj's rows: {gated}")
elif geometry_warnings(qproj, gated):
    bad.append(f"a gated q_proj on its own grid warned: {geometry_warnings(qproj, gated)}")
elif not geometry_warnings(qproj, plain):
    bad.append("the same q_proj is quiet without attn_output_gate, so the case proves nothing")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

npu: check_q4nx_dir calls every gated (attn_output_gate) pack a padded grid — the real d3 pack and all seven 27B qwen3_5 dirs fail --strict

1 participant